node: Correct the LocalSource check for npm provider #378

Ian2020 · 2023-10-13T13:10:28Z

Fix for #377. The check was comparing a string against a PosixPath which always fails. The intention is to see if the package_json file has any parent called "node_modules". Here's an example of what I mean:

@>>> from pathlib import Path
@>>> a = Path("./node_modules/ansi-gray")
@>>> "node_modules" in a.parents
False # WRONG
@>>> Path("node_modules") in a.parents
True # RIGHT

I wanted to write a test for this situation but was unsure how to structure it. I don't think the current tests in test_providers.py ever do a node install so there is no node_modules dir and thus this scenario is never tested. However I think it's more than likely to occur in reality.

refi64

I wanted to write a test for this situation but was unsure how to structure it.

We actually do have an existing test for installing local packages: see tests/data/local. However, the package-lock.v3.json inside it uses file::

{
  "name": "@flatpak-node-generator-tests/local",
  "version": "1.0.0",
  "lockfileVersion": 3,
  "requires": true,
  "packages": {
    "": {
      "name": "@flatpak-node-generator-tests/local",
      "version": "1.0.0",
      "dependencies": {
        "subdir": "file:subdir"
      }
    },
    "node_modules/subdir": {
      "name": "@flatpak-node-generator-tests/subdir",
      "version": "1.0.0",
      "resolved": "file:subdir"
    }
  }
}

so it ends up just following the file: branch later on, and thus this particular part wasn't tested. I...don't actually know what scenario results in this branch being taken, could you share the package-lock.json that reproduced the bug? (cc @gasinvein you might remember more about this than I do)

refi64 · 2023-10-13T16:36:51Z

node/flatpak_node_generator/providers/npm.py

@@ -110,7 +110,7 @@ def _process_packages_v2(
            source: PackageSource
            package_json_path = lockfile.parent / install_path / 'package.json'
            if (
-                'node_modules' not in package_json_path.parents
+                Path('node_modules') not in package_json_path.parents


OOF that's a nasty bug! If the intent is to see if any component is node_modules though, this also doesn't work:

>>> Path('node_modules') in Path('node_modules/x').parents True >>> Path('node_modules') in Path('a/node_modules/x').parents False >>>

Perhaps this should instead be:

Suggested change

Path('node_modules') not in package_json_path.parents

'node_modules' not in package_json_path.parts[:-1]

(not actually sure if the :-1 is needed to trim off the last part; the OG code used parents, but I dunno how important excluding the final component is)

Good catch - yes I'm not exactly sure if we're just trying to catch any dir called node_modules in the path or if it has to be the last/first. Maybe I'll see if there's a spec for this or something in the node docs.

Yeah this definitely should've been package_json_path.parts. My bad.

That works for me, I've made the change.

Ian2020 · 2023-10-14T09:11:25Z

My package-lock.json is nothing special I don't think (package-lock.json) - it's just that for many of my packages the second clause of the LocalSource test is already true (package_json_path.exists()):

            if (
                Path('node_modules') not in package_json_path.parents
                and package_json_path.exists()
            ):

...so thanks to the first clause already being defective most of my packages end up considered local.

Ian2020 · 2023-10-17T11:16:18Z

We actually do have an existing test for installing local packages...however, the package-lock.v3.json inside it uses file...so it ends up just following the file: branch later on, and thus this particular part wasn't tested. I...don't actually know what scenario results in this branch being taken

I've rethought my approach here - please take a look at the latest changes. I've got to a solution that's a bit simpler now and I think the existing tests cover all the scenarios adequately.

Your comment above was correct to question why this "LocalSource" branch is needed if the file: case is already handled in another branch. If I remove the LocalSource branch completely we get some test failures but only in tests that use package-lock.v3.json. Those failures (it turns out) are due to the fact that the code tries to process the "root" package as a dependency as well as all the "real" dependencies. To explain further here's part of a typical v3 package-lock:

...
  "packages": {
    "": {
      "name": "@flatpak-node-generator-tests/name-as-dep",
      "version": "1.0.0",
      "dependencies": {
        "word-wrap": "^1.2.3"
      }
    },
    "node_modules/word-wrap": {
      "version": "1.2.3",
      "resolved": "https://registry.npmjs.org/word-wrap/-/word-wrap-1.2.3.tgz",
      "integrity": "sha512-Hz/mrNwitNRh/HUAtM/VT/5VH+ygD6DV7mYKZAtHOrbs8U7lvPS6xf7EJKMF0uW1KJCl0H701g3ZGus+muE5vQ==",
      "license": "MIT",
      "engines": {
        "node": ">=0.10.0"
      }
    }
  }
...

That first package with an empty key is added by npm for the "root" project:

The root project is typically listed with a key of "", and all other packages are listed with their relative paths from the root project folder.

From package-lock.json.

This root project entry causes problems for _process_packages_v2 as it mistakenly processes it as if it were a real dependency. Previously the LocalSource branch was catching this dependency, misclassifying the root project as a LocalSource and thus it was ignored, by not by intention.

If instead we add code to ignore the root package explicitly all the tests pass and we can remove the LocalSource branch completely.

I think this is a better approach and more clearly deals with the issue.

refi64 · 2023-11-03T16:38:42Z

Ah, nice investigation! A few small notes:

It's not entirely clear to me why the test was reverted?
It would make the history a bit cleaner if the commits were squashed into two, e.g.:
```
node: Correct the LocalSource check for npm
node: Explicitly ignore the root project
```
with the first containing the Cleaner expression commit as well. (Normally I'd just do a squash-on-merge or similar, but in this case there's two separate changes, so squashing on merge would collapse them into 1 which isn't quite what we want either.)

Ian2020 · 2023-11-07T12:53:16Z

Thanks!

It would make the history a bit cleaner...

I've squashed it down to the two you suggested, makes sense as it had got a bit messy. I've also fixed the poe blue format issue.

It's not entirely clear to me why the test was reverted?

I regretted adding the test initially but I've restored it now. It felt a bit weird to copy in a whole module (word-wrap) into the project just for a test but it does catch the problem effectively. If there's a better way to test this let me know.

dbrgn · 2024-07-31T22:45:05Z

Thanks @Ian2020 for the work on this, I'm also running into the problems in #377.

I applied the patch in this PR, but noticed that actual local dependencies (starting with file:) from a npm lockfile v3 cannot be handled anymore:

NotImplementedError: Don't know how to handle package libs/eslint-myplugin in ../../package-lock.json

When I only apply the first commit ("Correct the LocalSource check for npm"), but not the second one, it seems to work. (I run into #382, but that's another issue.)

Ian2020 · 2024-08-07T10:26:04Z

Hey @dbrgn thanks for the feedback, I'll take a look when I get some time.

See flatpak/flatpak-builder-tools#378

Ian2020 changed the title ~~Correct the LocalSource check for npm provider~~ node: Correct the LocalSource check for npm provider Oct 13, 2023

refi64 reviewed Oct 13, 2023

View reviewed changes

Ian2020 added 2 commits November 7, 2023 12:41

Correct the LocalSource check for npm

ca0bb80

Explicitly ignore the root project

4365f7d

Ian2020 force-pushed the npm branch from b256b12 to 4365f7d Compare November 7, 2023 12:47

hfiguiere added npm node labels Apr 5, 2024

threema-danilo added a commit to threema-ch/threema-desktop that referenced this pull request Dec 18, 2024

flatpak-node-generator: Apply patch for lockfile v3 local sources

8b1ae81

See flatpak/flatpak-builder-tools#378

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

node: Correct the LocalSource check for npm provider #378

node: Correct the LocalSource check for npm provider #378

Ian2020 commented Oct 13, 2023

refi64 left a comment

refi64 Oct 13, 2023

Ian2020 Oct 14, 2023

gasinvein Oct 14, 2023

Ian2020 Oct 14, 2023

Ian2020 commented Oct 14, 2023 •

edited

Loading

Ian2020 commented Oct 17, 2023

refi64 commented Nov 3, 2023

Ian2020 commented Nov 7, 2023

dbrgn commented Jul 31, 2024

Ian2020 commented Aug 7, 2024

	Path('node_modules') not in package_json_path.parents
	'node_modules' not in package_json_path.parts[:-1]

node: Correct the LocalSource check for npm provider #378

Are you sure you want to change the base?

node: Correct the LocalSource check for npm provider #378

Conversation

Ian2020 commented Oct 13, 2023

refi64 left a comment

Choose a reason for hiding this comment

refi64 Oct 13, 2023

Choose a reason for hiding this comment

Ian2020 Oct 14, 2023

Choose a reason for hiding this comment

gasinvein Oct 14, 2023

Choose a reason for hiding this comment

Ian2020 Oct 14, 2023

Choose a reason for hiding this comment

Ian2020 commented Oct 14, 2023 • edited Loading

Ian2020 commented Oct 17, 2023

refi64 commented Nov 3, 2023

Ian2020 commented Nov 7, 2023

dbrgn commented Jul 31, 2024

Ian2020 commented Aug 7, 2024

Ian2020 commented Oct 14, 2023 •

edited

Loading